-
-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adding kind and path attributes to suggest response object and using it in autocomplete #464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break projects depending on kiwix-serve suggestion endpoint. How many of them do we know about? Don't we have to preserve backward compatibility (for example by adding a new endpoint, say 'suggesturl')? I am just raising those questions to make sure that we didn't overlook anything.
src/server/internalServer.cpp
Outdated
@@ -427,7 +427,7 @@ std::unique_ptr<Response> InternalServer::handle_suggest(const RequestContext& r | |||
if (reader->hasFulltextIndex()) { | |||
MustacheData result; | |||
result.set("label", "containing '" + term + "'..."); | |||
result.set("value", term + " "); | |||
result.set("url", "search?content=" + bookName + "&pattern=" + term + "+"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't term
have to be URL-escaped here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veloman-yunkan right will do that......I should use kiwix::urlEncode for that purpose right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If kiwix::parseMimetypeCounter()
cannot be utilized for that purpose and/or you can make kiwix::urlEncode()
work with minimal tweaks, then yes, go for it.
P.S. 😉
static/skin/taskbar.js
Outdated
@@ -20,9 +20,15 @@ function htmlDecode(input) { | |||
} | |||
}, | |||
|
|||
focus: function(event, ui) { | |||
$( "#kiwixsearchbox" ).val(ui.item.label); | |||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My level of competence with jquery and DOM events is almost zero, so can you please explain why you return false
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default select and focus try to place ui.item.value
in the searchbox but we need to place ui.item.label
as our value now contains a URL, even if I write the line $( "#kiwixsearchbox" ).val(ui.item.label);
the autocomplete widget will continue with propagating the event ahead and will again change the value to ui.item.value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the doc, https://api.jqueryui.com/autocomplete/#option-source
label
is the value used in the drop down menu, value
is the value inserted in the input field.
It would be better to follow this convention as the suggest endpoint is mainly designed to be used by jqueryui.autocomplete.
We could keep the label
/value
as it is and add a kind
/path
/term
in the item.
This way we don't need a new focus
function, we don't break the api for other user of suggest
endpoint and we can correctly handle the item with our js (see other comments)
static/templates/suggestion.json
Outdated
@@ -1,7 +1,7 @@ | |||
[ | |||
{{#suggestions}}{{^first}},{{/first}} | |||
{ | |||
"value" : "{{value}}", | |||
"value" : "{{url}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url
will be HTML escaped/encoded here (in particular &
will be replaced with &
). That may seem wrong, but the current design of the full (front-end + back-end) system assumes that value
is HTML-decoded in the front-end. Please put a comment here making that explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what you mean here is I should write &
in place of &
in L430 InternalServer.cpp, other than that what comment should be added?
something like url received should be html encoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See http://mustache.github.io/mustache.5.html. The {{variable}}
syntax applies HTML escaping. For a URL that may seem wrong. The comment must say that this is intentional, since the receiving side in taskbar.js un-escapes this field.
You've took the wrong direction with this PR.
So when the user click on a suggestion, it is redirected to the article. The workflow should be :
I'm not aware of any projects using it. But @kelson42 or @rgaudin may know some. |
@mgautierfr What is in particular wrong about this PR? As far as I understand it is along the lines outlined in your proposed flow - now selecting a suggestion avoids sending a search request and directly goes to the suggested page. |
@MananJethwani Please rebase your branch and squash all commits into one. |
e97a89a
to
246c1be
Compare
@veloman-yunkan done |
@veloman-yunkan It is true that this will break compatibility. We know about a few of third party software dealing with this API. Considering that this should be easy to adapt their code and that basically how it was working before was buggy, I'm ready to break the backward compatibility. |
@mgautierfr Thx for the cristal clear explanation about how it works and should work! |
It is technically avoiding an unnecessary search yes. Here, we have the suggestion backend doing the search but also define what to do with the result (it computes the url to use). It would be to the frontend to get the raw results and build the corresponding url.
And even if we decide that the frontend is dumb and simply use what is computed by backend (It would be a valid design, although I prefer not to go this way), we are currently in the middle of the way. The frontend in the PR still have to add the On top of that, we now have two ways to do the search. One with the classical form (if user don't use completion system) and one with the js changing directly the windows' location. Not a big issue, but then we have one more thing to think about if we do a change. This PR is making the code more complex instead of simplifying it and reduce the reusability of the api. |
@mgautierfr so should I just change the response back to |
@mgautierfr please have a look now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgautierfr so should I just change the response back to label, value but this time keep the value as the article name and generate the URL in the frontend would that be fine? as that would make the PR follow the separation of concern you talk about.
This is the global idea yes. But see my comments in my review.
static/skin/taskbar.js
Outdated
@@ -20,9 +20,15 @@ function htmlDecode(input) { | |||
} | |||
}, | |||
|
|||
focus: function(event, ui) { | |||
$( "#kiwixsearchbox" ).val(ui.item.label); | |||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the doc, https://api.jqueryui.com/autocomplete/#option-source
label
is the value used in the drop down menu, value
is the value inserted in the input field.
It would be better to follow this convention as the suggest endpoint is mainly designed to be used by jqueryui.autocomplete.
We could keep the label
/value
as it is and add a kind
/path
/term
in the item.
This way we don't need a new focus
function, we don't break the api for other user of suggest
endpoint and we can correctly handle the item with our js (see other comments)
static/skin/taskbar.js
Outdated
item.value = encodeURI(htmlDecode(item.value)); | ||
|
||
if (item.label === `containing '${item.value}'...`) { | ||
item.value = `${root}/search?content=${bookName}&pattern=${item.value}+`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is a open questioning, not a asked change)
The +
(space) at the end of the request is to force a real search (avoid a redirection in the backend if the title exact matches https://github.com/kiwix/kiwix-lib/blob/master/src/server/internalServer.cpp#L514-L519).
If we remove the redirection in the backend, we don't need to a space at the end of the query.
As for suggestion, it may break some third party user (I don't know about any).
@kelson42 may have a opinion about this but I think we can remove this redirection in the backend and make the search
endpoint always do a search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of this PR must be updated
src/server/internalServer.cpp
Outdated
@@ -428,6 +430,8 @@ std::unique_ptr<Response> InternalServer::handle_suggest(const RequestContext& r | |||
MustacheData result; | |||
result.set("label", "containing '" + term + "'..."); | |||
result.set("value", term + " "); | |||
result.set("kind", "pattern"); | |||
result.set("path", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed to add path here as if a path was undefined for some part of suggestion mustache won't render
another way was to remove the path here and add
{{^path}}
"path" : ""
{{/path}}
in suggestion.json
other than that If I tried to remove the path from this line and not add above-mentioned part in suggestion.json
it won't render and errors won't appear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried finding if this is how conditioning works in mustache but couldn't find much, this works but might be a bit wrong
@mgautierfr @veloman-yunkan can you help me if this seems wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mustache format is documented here : https://mustache.github.io/mustache.5.html
12f8e8f
to
9626b03
Compare
static/templates/suggestion.json
Outdated
"kind" : "{{kind}}", | ||
{{#path}} | ||
"path" : "{{path}}" | ||
{{/path}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an empty path
this will result in an invalid JSON (a comma followed by a closing brace, which confuses the strict parsers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what should I do about it?....I confirm for empty path my current solution works fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks I understand what you meant now
dataType: "json", | ||
cache: false, | ||
|
||
response: function( event, ui ) { | ||
|
||
for(const item of ui.content) { | ||
item.label = htmlDecode(item.label); | ||
item.value = htmlDecode(item.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since path
is HTML-encoded in suggestion.json
(that is important for producing a valid JSON in case if path
contains a double quote symbol), it must be htmlDecode
-ed here, too.
fe25446
to
59e3faf
Compare
I approve this PR. |
sure |
59e3faf
to
f690098
Compare
@mgautierfr done, should I rebase it as well? |
humm... You've just done it no ? What do you want to rebase more ? |
sorry I meant squash |
Yes, you may. |
…it in autocomplete
f690098
to
5cb276a
Compare
Is this PR in Docker image now? |
@stardiviner No, we will need a new release of the libkiwix and a new release/build of the kiwix-tools (kiwix-serve) first. This will take a few additional weeks. |
Ok, Thanks for answering. @kelson42 |
@kelson42 @mgautierfr this resolves kiwix/kiwix-tools#205